ensure the default order of benchmarks is the same as declared in source code #1907
Conversation
…ethods by source code line number so the order is always the same
|
@AndreyAkinshin good point, I've addressed your feedback, PTAL |
| @@ -108,7 +115,6 @@ private static ImmutableConfig GetFullMethodConfig(MethodInfo method, ImmutableC | |||
| Tuple<MethodInfo, TargetedAttribute>[] iterationCleanupMethods) | |||
| { | |||
| return targetMethods | |||
| .Where(m => m.HasAttribute<BenchmarkAttribute>()) | |||
There was a problem hiding this comment.
It's a regression.
Currently, this code silently filters wrong methods. After this code will throw friendly message (and exit) if any method is not benchmarkMethod.
BenchmarkRunner.Run(typeof(BenchmarkClass), new MethodInfo[] { BenchmarkClass.PrivateMethod, string.Concat });There was a problem hiding this comment.
@YegorStepanov great catch, I've missed the fact that it's called by
public static BenchmarkRunInfo MethodsToBenchmarks(Type containingType, MethodInfo[] benchmarkMethods, IConfig config = null)which contains user input
There was a problem hiding this comment.
I'm adding checks for BenchmarkRunner now:
Should be BenchmarkRunner fast exit if any MethodInfo is wrong or it should print error to the console and keep running?
The same question for Type[]. (Note BenchmarkRunner.Run with args != null switches to BenchmarkSwitcher and exit with error)
BenchmarkRunner.Run(new Type[] {typeof(BenchmarkClass), typeof(int)}) There was a problem hiding this comment.
@YegorStepanov I think that for user provided MethodInfo it's OK to fail fast
The story behind this fix is quite long.
In the dotnet/performance repo we have a single type that allows for partitioning benchmarks. The idea is quite simple: it's a filter that takes partition index and count and performs modulo operation to check whether current benchmark belongs to given partition (and should be executed).
Entire logic:
Some time ago @DrewScoggins has realized that some of the benchmarks in our Reporting System seem to be missing data from time to time. In the example below you can see no new uploads since december:
I believe that it's most likely caused by lack of deterministic order in the reflection APIs (dotnet/runtime#46272). There is no guarantee that the value of
_counterwill be the same for the same benchmark if we compile and run exactly the same source code multiple times.Example: let's say that we have 4 benchmarks: A, B, C and D.
Usually they are provided to the filter (which uses their index for filtering) as A, B, C, D.
But sometimes they are ordered as A, B, D, C.
So if we have 2 partitions, and run the process with:
--partition-count 2 --partition-index 0: for ABCD it decides to run benchmark A and C
--partition-count 2 --partition-index 1: for ABDC it decides to run benchmark B and C.
As you can see, benchmark D was never executed because the index changed between two runs of exactly same code.
The types are already sorted by name:
BenchmarkDotNet/src/BenchmarkDotNet/Extensions/ReflectionExtensions.cs
Lines 132 to 138 in c3fb7b9
But we can't sort the methods by names as our users are used to the fact that BDN by default runs the benchmarks in the declaration order.
My proposal is to include the source code line provided by the compiler in the
[BenchmarkAttribute]and just use the value for ordering.I have not added any tests as it's super hard to repro this bug.
I am aware of the fact that the partition filter referenced above is fragile.